app/assets/javascripts/discourse/models/store.js.es6 (1)

192-192: Regex note: unnecessary backslash before _.

\_ in a JS regex is equivalent to _ since underscore is not a special character. Not a functional issue, just a readability nit.

Simplified regex

-      const m = /(.+)\_id(s?)$/.exec(k);
+      const m = /(.+)_id(s?)$/.exec(k);

app/assets/javascripts/discourse/adapters/rest.js.es6 (1)

22-22: String.replace with a string pattern only replaces the first occurrence.

type.replace('_', '-') will only replace the first underscore. This works today because current admin model names have at most one underscore, but it's fragile. Use a regex with the global flag instead.

♻️ Proposed fix

-    if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; }
+    if (ADMIN_MODELS.indexOf(type.replace(/_/g, '-')) !== -1) { return "/admin/"; }

test/javascripts/helpers/create-pretender.js.es6 (1)

226-233: Pre-existing: /fruits/:id always returns fruits[0] regardless of the requested id.

The handler ignores request.params.id and always returns the first fruit. This predates this PR but is worth noting since the test now requests id 2 yet receives id 1.

♻️ Optional fix to make the handler id-aware

-    this.get('/fruits/:id', function() {
-      const fruit = fruits[0];
+    this.get('/fruits/:id', function(request) {
+      const fruit = fruits.findBy('id', parseInt(request.params.id));
       return response({ __rest_serializer: "1", fruit, farmers, colors });
     });

spec/controllers/admin/embeddable_hosts_controller_spec.rb (1)

1-9: RuboCop: Remove extra empty lines inside the describe block.

Static analysis flags extra blank lines at the block body beginning (Line 4) and end (Line 8). This matches the Layout/EmptyLinesAroundBlockBody convention.

🔧 Proposed fix

 describe Admin::EmbeddableHostsController do
-
   it "is a subclass of AdminController" do
     expect(Admin::EmbeddableHostsController < Admin::AdminController).to eq(true)
   end
-
 end

spec/controllers/admin/embedding_controller_spec.rb (1)

1-9: RuboCop: Same empty line violations as the sibling spec.

Same Layout/EmptyLinesAroundBlockBody issues at Lines 4 and 8.

🔧 Proposed fix

 describe Admin::EmbeddingController do
-
   it "is a subclass of AdminController" do
     expect(Admin::EmbeddingController < Admin::AdminController).to eq(true)
   end
-
 end

app/serializers/embedding_serializer.rb (1)

4-7: Redundant id method — ApplicationSerializer already provides this.

The explicit def id; object.id; end duplicates the default behavior inherited from ApplicationSerializer. It can be safely removed unless the codebase has a convention of always defining it explicitly.

♻️ Suggested simplification

 class EmbeddingSerializer < ApplicationSerializer
   attributes :id
   has_many :embeddable_hosts, serializer: EmbeddableHostSerializer, embed: :ids
-
-  def id
-    object.id
-  end
 end

app/assets/javascripts/admin/templates/components/embeddable-host.hbs (1)

17-17: Nit: inconsistent quote style.

Line 17 uses single quotes (class='btn-danger') while the rest of the template uses double quotes. Consider aligning for consistency.

-    {{d-button icon="trash-o" action="delete" class='btn-danger'}}
+    {{d-button icon="trash-o" action="delete" class="btn-danger"}}

app/serializers/embeddable_host_serializer.rb (1)

4-14: All three accessor methods are redundant.

ApplicationSerializer (via ActiveModel::Serializer) auto-delegates declared attributes to object.<attr>. These explicit methods just repeat the default behavior and can be removed to reduce boilerplate.

♻️ Suggested simplification

 class EmbeddableHostSerializer < ApplicationSerializer
   attributes :id, :host, :category_id
-
-  def id
-    object.id
-  end
-
-  def host
-    object.host
-  end
-
-  def category_id
-    object.category_id
-  end
 end

spec/models/embeddable_host_spec.rb (1)

23-30: Describe block name doesn't match the method under test.

The describe block is named "allows_embeddable_host" but the method being tested is host_allowed?. Consider renaming for clarity:

-  describe "allows_embeddable_host" do
+  describe ".host_allowed?" do

Also, the test named 'works as expected' is generic. More descriptive names (e.g., "returns true for allowed hosts and false for unlisted hosts") improve spec readability and failure diagnostics.

app/assets/javascripts/admin/controllers/admin-embedding.js.es6 (1)

5-7: saveChanges has no error handling and sends an empty payload.

update({}) returns a promise, but rejected responses are silently swallowed. The user receives no feedback on failure.

Suggested improvement

     saveChanges() {
-      this.get('embedding').update({});
+      this.get('embedding').update({}).catch(popupAjaxError);
     },

This requires importing popupAjaxError at the top of the file (as done in the sibling component):

import { popupAjaxError } from 'discourse/lib/ajax-error';

app/models/embeddable_host.rb (1)

10-18: record_for_host returns false instead of nil on failure — inconsistent with .first return.

Lines 12 and 15 return false, but line 17 (.first) returns nil when no record matches. Callers using .present? (like host_allowed?) are unaffected, but any caller checking record_for_host(url) with a nil-check (e.g., in TopicEmbed.import which uses eh.try(...)) could behave unexpectedly since false.try(:category_id) returns nil while the intent is "no record found."

Suggested fix: return nil consistently

   def self.record_for_host(host)
     uri = URI(host) rescue nil
-    return false unless uri.present?
+    return nil unless uri.present?

     host = uri.host
-    return false unless host.present?
+    return nil unless host.present?

     where("lower(host) = ?", host).first
   end

app/assets/javascripts/admin/components/embeddable-host.js.es6 (1)

43-51: Missing error handling on destroyRecord in delete action.

If the server-side deletion fails, the promise rejection is unhandled — the user sees no error feedback, and the host remains in the local collection (since sendAction only fires on success). Add a .catch handler for consistency with the save action.

Suggested fix

     delete() {
       bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => {
         if (result) {
           this.get('host').destroyRecord().then(() => {
             this.sendAction('deleteHost', this.get('host'));
-          });
+          }).catch(popupAjaxError);
         }
       });
     },

app/controllers/admin/embedding_controller.rb (1)

3-3: Remove redundant inherited callbacks in EmbeddingController.

Admin::AdminController already enforces ensure_logged_in and ensure_staff through its own before_filter declarations, which are inherited automatically by Admin::EmbeddingController. Only the :fetch_embedding callback needs to be explicitly declared here.

Suggested simplification

-  before_filter :ensure_logged_in, :ensure_staff, :fetch_embedding
+  before_filter :fetch_embedding

app/controllers/admin/embeddable_hosts_controller.rb (1)

22-32: No strong parameters — params are accessed directly.

params[:embeddable_host][:host] and params[:embeddable_host][:category_id] are read without whitelisting via params.require(:embeddable_host).permit(:host, :category_id). While Admin::AdminController restricts access to staff, using strong parameters is still a Rails best practice to guard against mass assignment and clearly document the accepted input surface.

Proposed refactor

  protected

+   def embeddable_host_params
+     params.require(:embeddable_host).permit(:host, :category_id)
+   end
+
    def save_host(host)
-     host.host = params[:embeddable_host][:host]
-     host.category_id = params[:embeddable_host][:category_id]
+     host.host = embeddable_host_params[:host]
+     host.category_id = embeddable_host_params[:category_id]
      host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank?

spec/fabricators/embeddable_host_fabricator.rb (1)

23-26: update! inside after_build will persist the record prematurely.

after_build runs before the record is saved. Calling cat.update!(read_restricted: true) here will force a database write, meaning even Fabricate.build(:private_category, group: g) will hit the DB — defeating the purpose of build. Consider using direct attribute assignment instead:

Proposed fix

  after_build do |cat, transients|
-   cat.update!(read_restricted: true)
+   cat.read_restricted = true
    cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full])
  end